Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Oct 2, 2024

I feel like using this API in other place will result in needing to duplicate a lot of strings, but not sure how to handle this otherwise. :/

@nielsdos
Copy link
Member

nielsdos commented Oct 2, 2024

To avoid duplications, make sure that we always use zend_strings for bv_val, which seems to be the case.
Then, just store ZSTR_VAL(str) inside bv_val, without duplicating or anything. Then also don't release the strings. Then, on cleanup instead of efree(bv_val) you can do something like this in pseudocode: zend_string_release(ptr - XtOffset(zend_string, val))

@Girgias
Copy link
Member Author

Girgias commented Oct 2, 2024

To avoid duplications, make sure that we always use zend_strings for bv_val, which seems to be the case. Then, just store ZSTR_VAL(str) inside bv_val, without duplicating or anything. Then also don't release the strings. Then, on cleanup instead of efree(bv_val) you can do something like this in pseudocode: zend_string_release(ptr - XtOffset(zend_string, val))

Okay this is ingenious, I like it, might have a helper function for this tho as it is slightly exotic

@nielsdos
Copy link
Member

nielsdos commented Oct 2, 2024

I agree with making a helper macro/function to make the conversion. I would also propose to add the macro only to ext/ldap and not to somewhere generic because this is very much a "beware there be dragons" macro.

@Girgias Girgias force-pushed the ldap-values-parsing branch from edba601 to 83b017a Compare October 2, 2024 23:51
@Girgias
Copy link
Member Author

Girgias commented Oct 2, 2024

I have implemented the suggestion and made some follow-up changes.

I think this can be used in more places, but I need to do some preliminary refactorings to be able to use.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks almost good, 2 nits.

ext/ldap/ldap.c Outdated
case IS_OBJECT:
return zval_try_get_string(zv);
case IS_TRUE:
return zend_string_init("TRUE", strlen("TRUE"), 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use ZSTR_INIT_LITERAL. Same for false.

ext/ldap/ldap.c Outdated
return -1;
}

bool control_iscritical = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use false instead of 0.

@Girgias Girgias force-pushed the ldap-values-parsing branch from 83b017a to 0968aa4 Compare October 3, 2024 21:43
@Girgias Girgias merged commit e01cde7 into php:master Oct 5, 2024
10 checks passed
@Girgias Girgias deleted the ldap-values-parsing branch October 5, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants